Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor depency handling, installation, and add a few features #9

Merged
merged 12 commits into from
Jun 28, 2020
Merged

Refactor depency handling, installation, and add a few features #9

merged 12 commits into from
Jun 28, 2020

Conversation

n42
Copy link
Contributor

@n42 n42 commented Jun 24, 2020

Hey,

Great idea and immediately useful, for me. I've done some quick and dirty - but fairly major - refactoring here as follows for my own sake - use it if you wish.

  • Got rid of the graphviz and pyyaml dependencies that were included in the repo
  • Add a setup.py script with some preliminary metadata and the required dependencies, in order to 'pip install' instead. It should be fairly easy to get it onto PyPi.
  • Updated documentation to reflect the above, rebuilt the demos
  • Broke out the argument parsing to a separate function and removed the defunct 'test' default input (the file you pointed to wasn't included in the repo, so running without an argument crashed rather than printed the help)
  • Feature: add a field for specifying exact cable SKUs and the like (specified_cable key)
  • Feature: add automatic AWG->mm2 conversion as well, when using show_equiv. Some minor refactoring there as well, modified ex06 to show it.

n42 added 9 commits June 24, 2020 20:20
@n42
refactor dependencies and installation
2d428e8 
There are hard copies of the graphviz and pyyaml dependencies included
in the repo. Remove these.

Sort out installation and dependency handling by writing a functional
setup.py script. Rename top level documentation. Refactor wireviz.py
slightly to allow it to run as an installed script.
@n42
clean up imports, cmdline handling
7af7567 
Order imports properly. Fix path when running script directly from repo
rather than as an installed package. Refactor command line handling.
@n42
add install instructions to readme
869454c
@n42
update build_examples script
2834392
@n42
update rebuilt examples/demos etc
121d664
@n42
Fix typo
23eaa07
@n42
Add 'specified_cable' field for BOM generation
653597a 
There is no obvious way of specifying exactly which cable type
(manufacturer, part number) to use for a given cable, which is useful
for BOM purposes. Add a 'specified_cable' field to the Cable class to
remedy this, and render it both in the graph and the BOM.
@n42
fix awg<->mm2 conversions
6c05537 
Add an inverted dictionary and a lookup function from awg -> mm2. Also
do some minor refactoring. Both sides of the conversion table were
converted to strings, since '0000' and '2/0' are perfectly valid AWG
values.
@n42
update example ex06 with awg -> mm2 conversion
e86d09f 
Show conversions for ex06, and make sure it displays conversions in both
directions. Rebuild the example files.
@SuperRoach
Copy link

I like this implementation to make it easier to use! One thing that could make it even easier is the first user setup.

  • User git clones repo

  • Either on bare metal installing python or spins up a dev container that has python3 preinstalled

  • does pip3 install -e .

  • python3 src/wireviz/wireviz.py examples/ex03.yml (run a sample file)

Will give a
Traceback (most recent call last): File "src/wireviz/wireviz.py", line 639, in <module> main() File "src/wireviz/wireviz.py", line 636, in main parse(args.input_file, file_out=args.output_file, gen_bom=args.generate_bom) File "src/wireviz/wireviz.py", line 615, in parse h.output(filename=file_out, format=('png','svg'), gen_bom=gen_bom, view=False) File "src/wireviz/wireviz.py", line 218, in output d.render(filename=filename, directory=directory, view=view, cleanup=cleanup) File "/usr/local/lib/python3.8/site-packages/graphviz/files.py", line 207, in render rendered = backend.render(self._engine, format, filepath, File "/usr/local/lib/python3.8/site-packages/graphviz/backend.py", line 221, in render run(cmd, capture_output=True, cwd=cwd, check=True, quiet=quiet) File "/usr/local/lib/python3.8/site-packages/graphviz/backend.py", line 167, in run raise ExecutableNotFound(cmd) graphviz.backend.ExecutableNotFound: failed to execute ['dot', '-Tpng', '-O', 'ex03'], make sure the Graphviz executables are on your systems' PATH

I'm guessing it's because the png and svg libraries it's calling arn't there yet.

@n42
Copy link
Contributor Author

n42 commented Jun 25, 2020

I like this implementation to make it easier to use! One thing that could make it even easier is the first user setup.

  • User git clones repo
  • Either on bare metal installing python or spins up a dev container that has python3 preinstalled
  • does pip3 install -e .
  • python3 src/wireviz/wireviz.py examples/ex03.yml (run a sample file)

Will give a
Traceback (most recent call last): File "src/wireviz/wireviz.py", line 639, in <module> main() File "src/wireviz/wireviz.py", line 636, in main parse(args.input_file, file_out=args.output_file, gen_bom=args.generate_bom) File "src/wireviz/wireviz.py", line 615, in parse h.output(filename=file_out, format=('png','svg'), gen_bom=gen_bom, view=False) File "src/wireviz/wireviz.py", line 218, in output d.render(filename=filename, directory=directory, view=view, cleanup=cleanup) File "/usr/local/lib/python3.8/site-packages/graphviz/files.py", line 207, in render rendered = backend.render(self._engine, format, filepath, File "/usr/local/lib/python3.8/site-packages/graphviz/backend.py", line 221, in render run(cmd, capture_output=True, cwd=cwd, check=True, quiet=quiet) File "/usr/local/lib/python3.8/site-packages/graphviz/backend.py", line 167, in run raise ExecutableNotFound(cmd) graphviz.backend.ExecutableNotFound: failed to execute ['dot', '-Tpng', '-O', 'ex03'], make sure the Graphviz executables are on your systems' PATH

I'm guessing it's because the png and svg libraries it's calling arn't there yet.

Ah, apart from the graphviz python package (bindings) it also requires the actual graphviz system package; pip doesn't pull that for you. Already had it installed, so didn't catch that -- it's a documentation issue, i'll drop in a fix in the install notes and add to the PR, but an apt-get install graphviz (or your equivalent package manager command if running anything other than debian derivatives) should sort you out, @SuperRoach

@n42
fix install notes
cb1d6cd 
Explicitly mention the system graphviz dependency and clarify possible
need for separate python install on old ubuntus
@n42
Copy link
Contributor Author

n42 commented Jun 25, 2020

cb1d6cd fixes the install instructions as above; verified that no other dependencies are required using a clean dockerized ubuntu.

n42 added 2 commits June 25, 2020 13:40
@n42
feature: add --prepend-file option
1f34651 
- Allow prepending a separate YAML file for e.g. including common
  template definitions. This is accomodated by a new commandline option,
  --prepend-file, which takes a path to a YAML file. This is prepended
  to the regular input as-is.

- Refactor file loading to accomodate the above. This includes moving
  relevant parts to main() and instead supplying parse () with a string
  representation of the YAML data. Also add early file existance checks
  and bail out if any of the inputs are inaccessible or nonexistant.
@n42
refactor: rename 'input' -> 'yaml_data'
07e9f78 
'input' overloads a python built-in name. Refactor to avoid this.
@formatc1702
Copy link
Collaborator

This looks great! I'll try to merge this in the next few days :)

@formatc1702
Copy link
Collaborator

What would be the best way to split this into multiple merges, to separate the dependency handling + installation from the added features?
This is my first time managing a collaborative project like this, so any pointers are appreciated!

@Cabalist Cabalist mentioned this pull request Jun 28, 2020
Closed
@formatc1702 formatc1702 changed the base branch from master to refactor June 28, 2020 08:28
@formatc1702 formatc1702 merged commit 967c4a7 into wireviz:refactor Jun 28, 2020
@formatc1702
Copy link
Collaborator

I've cherry picked the commits relevant to dependency handling and refactoring, skipping the added features for now. Will push once I confirm everything works.

@formatc1702
Copy link
Collaborator

Integrated into dev branch with 7060c38 , please confirm that everything went smoothly! I made some minor changes to make the build_examples.py script work again.

This was referenced Jun 28, 2020
Closed
Closed
@n42 n42 mentioned this pull request Jun 29, 2020
Merged
@formatc1702
Copy link
Collaborator

formatc1702 commented Jun 29, 2020 •
edited
Loading

I've added the project to PyPI 🎉 There's plenty of things to learn and improve, but at least it's out there.

Any further assistance on this topic will be highly appreciated, as I have zero previous experience with PyPI and the intricacies of packaging a project...

@kvid kvid mentioned this pull request Oct 12, 2020
Closed
@obelisk79 obelisk79 mentioned this pull request Jun 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@n42 @SuperRoach @formatc1702